🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 1 finding(s).

Panel: 5/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), claude-opus-4-8-thinking-xhigh:edge-case (parse_error), kimi-k2.5:edge-case (empty)

Requesting changes on one line — everything else here is verified and good, including a security fix that's better than the description lets on.

Blocking: this PR turns main's ruff_check red

ruff_check is failing, and it's PR-caused, not stale CI. The branch rewrites an already-correctly-formatted docstring into a form ruff rejects:

-        """A rate-limited 403 surfaces as GitHubRateLimitError, not a silent "no PR found\""""
+        """A rate-limited 403 surfaces as GitHubRateLimitError, not a silent "no PR found\" """

Proof it's this branch and not the environment — same ruff, both trees, this branch merged with current main:

$ uvx ruff@0.15.15 format --diff .    # on origin/main
270 files already formatted

$ uvx ruff@0.15.15 format --diff .    # on this branch
1 file would be reformatted

Why it passed for you: CI pins ruff 0.15.15 (.github/workflows/ruff_check.yml) and runs ruff format --diff, but 0.15.13 accepts this line — I hit the same false-green locally until I matched the pin. Your "ruff check clean" claim is literally true; it's ruff format that fails. Worth running uvx ruff@0.15.15 format --diff . before pushing.

Revert that one line to main's version and this is ready — nothing else needs to change.

The -- fix is a real RCE guard, and it isn't in the description at all

Commit 667887d (-- separator on git fetch) is the most valuable thing in this PR and the body never mentions it. I verified the vulnerability is real against live git 2.39.5, using a branch name of --upload-pack=touch <path>:

main's form:  git fetch origin '--upload-pack=touch …'
              → command EXECUTED (marker file created)

this branch:  git fetch origin -- '--upload-pack=touch …'
              → fatal: invalid refspec '--upload-pack=touch …'   (blocked)

pr_info.head_branch comes straight from the GitHub API and flows into comfy install --pr against forks, and git check-ref-format accepts refs/heads/--upload-pack=x, so the CLI can't lean on git or GitHub refusing the name. This is worth its own line in the description (and arguably its own PR) so it isn't buried in a refactor.

I checked the calls you did not add -- to, and they're all safe by construction: the two git checkout -B start-points are prefixed (origin/…, remote/…), local_branch is prefixed pr-, and git remote add <name> <url> rejects a dash-leading URL with error: unknown option before doing anything (verified). So the two fetch calls were the whole hole.

Also confirmed git fetch <remote> -- <branch> is accepted by git — the tests mock subprocess.run, so nothing in the suite would have caught it if the syntax were wrong.

The cwd= refactor checks out

Verified against real git repos, not mocks:

• real repo + tag → returns True, process cwd unchanged
• missing tag → returns False via the shared panel
• both panels render; checkout_pr's is byte-identical as claimed

Fixing the Text.append(f"[cyan]{tag}[/cyan]") bug is a genuine improvement — Text.append doesn't parse markup, so users really were seeing literal [cyan]v1.2.3[/cyan]. And because Text.append doesn't parse markup, a tag or PR title containing [...] can't raise MarkupError through the new helper.

One description nit: the body says cwd= "removes the failure mode where os.chdir(repo_path) itself throws (e.g. missing dir)". It doesn't — the throw just moves from os.chdir to subprocess.run(cwd=…). Both main and this branch raise an uncaught FileNotFoundError for a missing repo dir; I checked both. No regression, but the claim overstates it — what you actually removed is the process-global cwd mutation, which is a good enough reason on its own.

(Unrelated and pre-existing on both branches: import comfy_cli.git_utils on its own raises ImportError from a cycle with comfy_cli/command/github/__init__.py. Not yours, not in scope — just noting it since it bites anyone importing the module directly.)

Fixed — clearing my earlier changes-requested.

The docstring is now byte-identical to main:

this branch: 183:        """A rate-limited 403 … not a silent "no PR found\""""
main:        183:        """A rate-limited 403 … not a silent "no PR found\""""

and CI's pinned toolchain is clean on both halves:

$ uvx ruff@0.15.15 check .          -> All checks passed!
$ uvx ruff@0.15.15 format --diff .  -> 272 files already formatted

The fix commit touched exactly 1 file, 1 line — nothing else moved. Both git fetch … -- guards are still in place (git_utils.py:128 and :150), so the argument-injection fix I verified earlier is intact: a branch named --upload-pack=touch <path> executed on main's form and is rejected as an invalid refspec here.

Full suite re-run green on this branch merged with current main.

Everything from the earlier review stands — the cwd= refactor checked out against real git repos (process cwd unchanged, False on a bad tag, both panels rendering), and the Text.append markup bug fix is a genuine improvement.

Worth acting on the root cause I posted separately so this doesn't recur: your local ruff is 0.14.x, which rewrites that docstring automatically; 0.15.x leaves it alone. Pinning to ruff==0.15.15 locally will stop it re-appearing in any PR that touches a file with a docstring ending in an escaped quote.